util: use a global symbol for `util.promisify.custom` #31672

ExE-Boss · 2020-02-07T10:44:09Z

Define util.promisify.custom as:

Symbol.for("nodejs.util.inspect.custom")

rather than as:

Symbol("util.inspect.custom")

This allows custom promisify wrappers to easily/safely be defined in non‑Node.js environments and for non‑Node promisify implementations to be interoperable.

Fixes: #31647

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Define `util.promisify.custom` as `Symbol.for("nodejs.util.inspect.custom")`, rather than as `Symbol("util.inspect.custom")`. This allows custom `promisify` wrappers to easily/safely be defined in non‑Node.js environments. Fixes: nodejs#31647

addaleax · 2020-02-07T10:52:15Z

@ExE-Boss Do you think you could add documentation/tests here? You could take a look at dadd6e1 for inspiration. Basically, there should be one example of using this way of accessing the symbol in the docs, and one in the tests.

ExE-Boss · 2020-02-07T10:52:55Z

I’m waiting for CI to finish, so that I know which tests to update.

nodejs-github-bot · 2020-02-07T11:39:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28980/

lib/internal/util.js

jasnell · 2020-02-07T14:52:07Z

doc/api/util.md


+In addition to being accessible through `util.promisify.custom`, this
+symbol is [registered globally][global symbol registry] and can be
+accessed in any environment as `Symbol.for('nodejs.util.promisify.custom')`.


It should be noted that any code that uses Symbol.for('nodejs.util.promisify.custom') syntax rather than the util.promisify.custom accessor will not work properly on older versions of Node.js so if compatibility with older versions is important, use of the accessor is preferred.

That will be visible in the History drop‑down, same as with util.inspect.custom, which doesn’t mention this either.

I'd prefer a more explicit comment on it.

I recommend doing that in its own PR, as it will affect the documentation of util.inspect.custom (and possibly other symbols) as well.

ExE-Boss · 2020-02-08T00:16:04Z

We should probably ensure that all methods which use the internal customPromisifyArgs symbol get passed to util.promisify before any user code runs, to ensure that any shims that use Symbol.for("nodejs.util.promisify.custom") do not break built‑in functions, ex.: @browserify’s util shim, which doesn’t even implement the customPromisifyArgs codepath, or @ljharb’s util.promisify, which exposes its own version of the customPromisifyArgs symbol and associated codepath, which is incompatible with Node’s.

Define `util.promisify.custom` as `Symbol.for(nodejs.util.inspect.custom)`, rather than as `Symbol(util.inspect.custom)`. This allows custom `promisify` wrappers to easily/safely be defined in non‑Node.js environments. Refs: nodejs/node#31647 Refs: nodejs/node#31672

ljharb · 2020-02-08T05:21:41Z

My util.promisify package only exposes a new symbol if util.promisify.custom doesn't already exist; there's no issue here.

Define `util.promisify.custom` as `Symbol.for(nodejs.util.inspect.custom)`, rather than as `Symbol(util.inspect.custom)`. This allows custom `promisify` wrappers to easily/safely be defined in non‑Node.js environments. Refs: nodejs/node#31647 Refs: nodejs/node#31672

Define `util.promisify.custom` as `Symbol.for("nodejs.util.inspect.custom")`, rather than as `Symbol("util.inspect.custom")`. This allows custom `promisify` wrappers to easily/safely be defined in non‑Node.js environments. Refs: nodejs/node#31647 Refs: nodejs/node#31672

ExE-Boss · 2020-03-16T22:53:05Z

Should this be backported to v12.x and v13.x just like how #20857 (comment) was?

Define `util.promisify.custom` as `Symbol.for("nodejs.util.inspect.custom")`, rather than as `Symbol("util.inspect.custom")`. This allows custom `promisify` wrappers to easily/safely be defined in non‑Node.js environments. Fixes: #31647 PR-URL: #31672 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Notable changes: * [a44da56] - (SEMVER-MINOR) doc: update stability of report features (Colin Ihrig) #32242 * [306ed96] - (SEMVER-MINOR) doc,lib,src,test: make --experimental-report a nop (Colin Ihrig) #32242 * [ea7f89d] - (SEMVER-MINOR) test: remove common.skipIfReportDisabled() (Colin Ihrig) #32242 * [3f1f518] - (SEMVER-MINOR) build: make --without-report a no-op (Colin Ihrig) #32242 * [36ab39f] - (SEMVER-MINOR) build: remove node_report option in node.gyp (Colin Ihrig) #32242 * [514b7c2] - (SEMVER-MINOR) src: unconditionally include report feature (Colin Ihrig) #32242 * [435fbbc] - (SEMVER-MINOR) worker: allow URL in Worker constructor (Antoine du HAMEL) #31664 * [975d6b0] - (SEMVER-MINOR) util: use a global symbol for `util.promisify.custom` (ExE Boss) #31672

Define `util.promisify.custom` as `Symbol.for("nodejs.util.inspect.custom")`, rather than as `Symbol("util.inspect.custom")`. This allows custom `promisify` wrappers to easily/safely be defined in non‑Node.js environments. Fixes: #31647 PR-URL: #31672 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Notable changes: * build: * macOS package notarization (Rod Vagg) #31459 * deps: * update npm to 6.14.3 (Myles Borins) #32368 * update to uvwasi 0.0.6 (Colin Ihrig) [#32309](#32309) * upgrade to libuv 1.35.0 (Colin Ihrig) #32204 * node\_report: * move diagnostic reports to stable (Colin Ihrig) #32242 * worker: * allow URL in Worker constructor (Antoine du HAMEL) #31664 * util: * use a global symbol for `util.promisify.custom` (ExE Boss) #31672 PR-URL: #32376

macOS package notarization and a change in builder configuration: The macOS binaries for this release, and future 13.x releases, are now being compiled on macOS 10.15 (Catalina) with Xcode 11 to support package notarization, a requirement for installing on .pkg files on macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being compiled to support a minimum of macOS 10.10 (Yosemite) we do not anticipate this having a negative impact on Node.js 13.x users with older versions of macOS. Notable changes: * build: * macOS package notarization (Rod Vagg) #31459 * deps: * update npm to 6.14.3 (Myles Borins) #32368 * update to uvwasi 0.0.6 (Colin Ihrig) #32309 * upgrade to libuv 1.35.0 (Colin Ihrig) #32204 * lib: * add --disable-proto option to cli (Gus Caplan) #32279 * node_report: * move diagnostic reports to stable (Colin Ihrig) #32242 * worker: * allow URL in Worker constructor (Antoine du HAMEL) #31664 * util: * use a global symbol for `util.promisify.custom` (ExE Boss) #31672 PR-URL: #32376

macOS package notarization and a change in builder configuration: The macOS binaries for this release, and future 13.x releases, are now being compiled on macOS 10.15 (Catalina) with Xcode 11 to support package notarization, a requirement for installing on .pkg files on macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being compiled to support a minimum of macOS 10.10 (Yosemite) we do not anticipate this having a negative impact on Node.js 13.x users with older versions of macOS. Notable changes: * build: * macOS package notarization (Rod Vagg) #31459 * deps: * upgrade npm to 6.14.4 (Ruy Adorno) #32495 * update to uvwasi 0.0.6 (Colin Ihrig) #32309 * upgrade to libuv 1.35.0 (Colin Ihrig) #32204 * lib: * add --disable-proto option to cli (Gus Caplan) #32279 * node_report: * move diagnostic reports to stable (Colin Ihrig) #32242 * worker: * allow URL in Worker constructor (Antoine du HAMEL) #31664 * util: * use a global symbol for `util.promisify.custom` (ExE Boss) #31672 PR-URL: #32376

Define `util.promisify.custom` as `Symbol.for("nodejs.util.inspect.custom")`, rather than as `Symbol("util.inspect.custom")`. This allows custom `promisify` wrappers to easily/safely be defined in non‑Node.js environments. Fixes: nodejs#31647 PR-URL: nodejs#31672 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Define `util.promisify.custom` as `Symbol.for("nodejs.util.inspect.custom")`, rather than as `Symbol("util.inspect.custom")`. This allows custom `promisify` wrappers to easily/safely be defined in non‑Node.js environments. Fixes: #31647 Backport-PR-URL: #32349 PR-URL: #31672 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Define `util.promisify.custom` as `Symbol.for("nodejs.util.inspect.custom")`, rather than as `Symbol("util.inspect.custom")`. This allows custom `promisify` wrappers to easily/safely be defined in non‑Node.js environments. Refs: nodejs/node#31647 Refs: nodejs/node#31672

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Feb 7, 2020

addaleax added lts-watch-v12.x semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 7, 2020

ExE-Boss added 2 commits February 7, 2020 12:16

docs(util): Use a global symbol for util.promisify.custom

1798266

test(util): Use a global symbol for util.promisify.custom

e7d919e

ExE-Boss changed the title ~~util: Use a global symbol for util.promisify.custom~~ util: use a global symbol for util.promisify.custom Feb 7, 2020

ExE-Boss requested review from BridgeAR, TimothyGu, addaleax and targos February 7, 2020 11:35

addaleax approved these changes Feb 7, 2020

View reviewed changes

cjihrig approved these changes Feb 7, 2020

View reviewed changes

jasnell reviewed Feb 7, 2020

View reviewed changes

lib/internal/util.js Show resolved Hide resolved

jasnell reviewed Feb 7, 2020

View reviewed changes

devsnek approved these changes Feb 7, 2020

View reviewed changes

ExE-Boss mentioned this pull request Feb 8, 2020

[New] use a global symbol for util.promisify.custom ljharb/util.promisify#19

Merged

ExE-Boss requested a review from ljharb February 8, 2020 00:22

ExE-Boss mentioned this pull request Feb 8, 2020

Need to patch the standard library ljharb/util.promisify#4

Open

ExE-Boss mentioned this pull request Feb 8, 2020

feat: use a global symbol for util.promisify.custom browserify/node-util#44

Open

BridgeAR approved these changes Feb 9, 2020

View reviewed changes

addaleax closed this Mar 12, 2020

ExE-Boss deleted the lib/util/use-global-util-promisify-custom-symbol branch March 13, 2020 02:32

ExE-Boss mentioned this pull request Mar 18, 2020

[v12.x backport] util: use a global symbol for util.promisify.custom #32349

Closed

4 tasks

MylesBorins mentioned this pull request Mar 19, 2020

v13.12.0 proposal #32376

Merged

codebytere mentioned this pull request Apr 3, 2020

v12.16.2 proposal #32313

Merged

MylesBorins removed the lts-watch-v12.x label Nov 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

util: use a global symbol for `util.promisify.custom` #31672

util: use a global symbol for `util.promisify.custom` #31672

Uh oh!

ExE-Boss commented Feb 7, 2020 •

edited

Loading

Uh oh!

addaleax commented Feb 7, 2020

Uh oh!

ExE-Boss commented Feb 7, 2020

Uh oh!

nodejs-github-bot commented Feb 7, 2020

Uh oh!

Uh oh!

jasnell Feb 7, 2020

Uh oh!

ExE-Boss Feb 7, 2020 •

edited

Loading

Uh oh!

jasnell Feb 7, 2020

Uh oh!

ExE-Boss Feb 8, 2020

Uh oh!

ExE-Boss commented Feb 8, 2020 •

edited

Loading

Uh oh!

ljharb commented Feb 8, 2020

Uh oh!

ExE-Boss commented Mar 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

util: use a global symbol for util.promisify.custom #31672

util: use a global symbol for util.promisify.custom #31672

Uh oh!

Conversation

ExE-Boss commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax commented Feb 7, 2020

Uh oh!

ExE-Boss commented Feb 7, 2020

Uh oh!

nodejs-github-bot commented Feb 7, 2020

Uh oh!

Uh oh!

jasnell Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

ExE-Boss Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

ExE-Boss Feb 8, 2020

Choose a reason for hiding this comment

Uh oh!

ExE-Boss commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Feb 8, 2020

Uh oh!

ExE-Boss commented Mar 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

util: use a global symbol for `util.promisify.custom` #31672

util: use a global symbol for `util.promisify.custom` #31672

ExE-Boss commented Feb 7, 2020 •

edited

Loading

ExE-Boss Feb 7, 2020 •

edited

Loading

ExE-Boss commented Feb 8, 2020 •

edited

Loading